For the record, this patch breaks part of the irq_lock() specification when CONFIG_SPIN_VALIDATE=y since there's no way to distinguish between k_spin_lock() and irq_lock():

Using QEMU, I verified that the existing assertion (when CONFIG_USE_SWITCH=y, from bd07756) does panic if the irq_lock() is held upon context switch:

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index c550ab461cb..be87d15501c 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,10 +5,13 @@
  */
 
 #include <stdio.h>
+#include <zephyr/kernel.h>
 
 int main(void)
 {
+       irq_lock();
        printf("Hello World! %s\n", CONFIG_BOARD_TARGET);
+       k_msleep(1000);
 
        return 0;
 }

$ # with upstream
$ west build -b qemu_x86_64 samples/hello_world/ -DCONFIG_USE_SWITCH=y -DCONFIG_ASSERT=y -DCONFIG_SPIN_VALIDATE=y -p
$ west build -t run
-- west build: running target run
[3/4] To exit from QEMU enter: 'CTRL+a, x'[QEMU] CPU: qemu64,+x2apic
qemu-system-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]
qemu-system-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]
SeaBIOS (version zephyr-v1.0.0-0-g31d4e0e-dirty-20200714_234759-fv-az50-zephyr)
Booting from ROM..
*** Booting Zephyr OS build v4.2.0-334-g124fb897b490 ***
Hello World! qemu_x86_64/atom
ASSERTION FAIL [arch_irq_unlocked(key) || z_smp_current_get()->base.thread_state & (((1UL << (0))) | ((1UL << (3))))] @ WEST_TOPDIR/zephyr/kernel/include/kswap.h:98
        Context switching while holding lock!
RAX: 0x0000000000000004 RBX: 0x000000000011fd90 RCX: 0x0000000000000001 RDX: 0x0000000000000000
RSI: 0x0000000000000062 RDI: 0x00000000001090d2 RBP: 0x000000000012bd70 RSP: 0x000000000012bd48
 R8: 0x0000000000000000  R9: 0x0000000000000000 R10: 0x0000000000000002 R11: 0x0000000000000000
R12: 0x000000000010b500 R13: 0x0000000000000000 R14: 0x0000000000000000 R15: 0x000000000012be48
RSP: 0x000000000012bd48 RFLAGS: 0x0000000000000002 CS: 0x0018 CR3: 0x0000000000136000
RIP: 0x000000000010046f
call trace:
     0: 0x00000000001051d5
     1: 0x000000000010524d
     2: 0x0000000000100025
     3: 0x0000000000102f2d
     4: 0x000000000010045a

On a related note, the k_spin_lock() documentation does not mention whether or not holding a k_spinlock is allowed upon reschedule:

The options I see are:

1. don't merge this patch ( 😢 )
2. merge patch as-is

• Holders of irq_lock on context switch will inexplicably panic if CONFIG_SPIN_VALIDATE=y
• ...and it should be remarked that CONFIG_SPIN_VALIDATE usually defaults to y if CONFIG_ASSERT=y

4. update documentation of irq_lock() to indicate that holding irq_lock on context switch will panic when CONFIG_SPIN_VALIDATE=y
5. update documentation of irq_lock() to no longer allow holding irq_lock on context switch (breaking change?)

My vote would just be to remove that section of the docs and rip the bandaid off with the warning (which can always be disabled via kconfig anyway). There are precisely zero circumstances where breaking a critical section (!) lock with a context switch (!!) cannot lead to tears.

Code that does that is not only breaking its own carefully curated lock in an immensely clever way that it clearly thought carefully about and knows has no unexpected interactions. Because of the recursive behavior of irq_lock() it's also breaking the locks taken by all the callers up the stack, who had no idea that calling into this obnoxiously clever subsystem was a synchronization trap.

It just can't work. We never should have documented it (and I didn't even know we had!).

What do you think of replacing the existing note:

with the following?

 * @note
 * This routine can be called by ISRs or by threads.
 * Only isr-ok functions may be called while holding the interrupt lock.
 * It is a fatal error to hold the interrupt lock upon return from ISR.

These statements sound correct from my understanding of the kernel:

• isr-ok functions are the only ones guaranteed to not context switch
• Kernel might reschedule upon return-from-ISR

We could add a more direct It is a fatal error to hold the interrupt lock upon context switch but I feel like these statements are equivalent, while being easier to understand from people not necessarily familiar with kernel terminology.

If this is fine for you, I'll add a new commit that edits irq.h.

It is a fatal error to hold the interrupt lock upon context switch

How about the infamous Linux wording BUG: Scheduling while atomic?

It is a fatal error to hold the interrupt lock upon context switch

How about the infamous Linux wording BUG: Scheduling while atomic?

We already have a runtime message 🙂

What needs updating is the documentation for irq_lock() as it isn't consistent with the rest of the kernel.

I think we need the explicit wording of "It is a fatal error to hold the interrupt lock upon context switch", since a thread can force a reschedule/context switch at any time by calling k_yield() for example.

I think we need the explicit wording of "It is a fatal error to hold the interrupt lock upon context switch", since a thread can force a reschedule/context switch at any time by calling k_yield() for example.

I don't think k_yield() is isr-ok though?

(Really, my Only isr-ok functions may be called while holding the interrupt lock is a non-kernel-wording way of saying It is a fatal error to hold the interrupt lock upon context switch - I don't mind adding it anyways if you feel like it's better though)

I think we need the explicit wording of "It is a fatal error to hold the interrupt lock upon context switch", since a thread can force a reschedule/context switch at any time by calling k_yield() for example.

I don't think k_yield() is isr-ok though?

(Really, my Only isr-ok functions may be called while holding the interrupt lock is a non-kernel-wording way of saying It is a fatal error to hold the interrupt lock upon context switch - I don't mind adding it anyways if you feel like it's better though)

I disagree with the way the note is worded, since when I'm reading it, I'm reading the three lines as a strangely phrased paragraph, and in my mind auto-correcting it to make it make sense. Could you split the three notes into three notes, or phrase it as a paragraph?

 * @note This routine can be called by ISRs or by threads.
 * @note Only isr-ok functions may be called while holding the interrupt lock.
 * @note It is a fatal error to hold the interrupt lock upon return from ISR.

or

* @note This function can be called by ISRs and threads. After calling this
* routine, the caller is "holding the interrupt lock". While holding the interrupt lock,
* only isr-ok functions may be called, as these don't result in context switches.
* While holding the interrupt lock, a context switch, which could result from calling
* a non isr-ok function, or returning from an ISR, is a fatal error.

Bikeshed aside, I think adding a warning in the docs is fine, but IMHO it should be as short and clear as possible. "Context switching while holding the irq_lock is illegal." is fine, etc... The user will be alerted by the assert if they trip over it. (Also nitpick: it's not a "fatal error" as nothing actually fails or panics in an obvious way, which is why this is a booby trap.)

Modified the irq_lock() (and k_spin_lock() while at it) documentation with short comments.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud